Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

accelerate core ruby requires #1113

Merged
merged 1 commit into from
Nov 13, 2019
Merged

accelerate core ruby requires #1113

merged 1 commit into from
Nov 13, 2019

Conversation

lamont-granquist
Copy link
Contributor

@lamont-granquist lamont-granquist requested review from a team as code owners November 1, 2019 23:58
@lamont-granquist
Copy link
Contributor Author

This is a 15% speedup in chef --version, not as impactful to chef-client/knife with all the require_relative and requires guards:

with patch:

% time /opt/chef/bin/knife --version
Chef Infra Client: 15.4.58
/opt/chef/bin/knife --version  0.60s user 0.11s system 97% cpu 0.734 total
% time /opt/chef/bin/chef-client --version
Chef Infra Client: 15.4.58
/opt/chef/bin/chef-client --version  1.61s user 0.22s system 98% cpu 1.867 total

% time /opt/chefdk/bin/chef --version
ChefDK version: 4.4.26
Chef Infra Client version: 15.3.14
Chef InSpec version: 4.16.0
Test Kitchen version: 2.3.3
Foodcritic version: 16.1.1
Cookstyle version: 5.6.2
/opt/chefdk/bin/chef --version  6.04s user 2.63s system 98% cpu 8.764 total


without patch:

i12:26 crash chef/omnibus % time /opt/chef/bin/knife --version
Chef Infra Client: 15.4.58
/opt/chef/bin/knife --version  0.70s user 0.11s system 97% cpu 0.837 total
git:chef:master↓✗ rvm:ruby-2.5.3
12:28 crash chef/omnibus % time /opt/chef/bin/chef-client --version
Chef Infra Client: 15.4.58
/opt/chef/bin/chef-client --version  1.65s user 0.26s system 98% cpu 1.935 total

i3:29 trimix chef-dk/omnibus % time /opt/chefdk/bin/chef --version
ChefDK version: 4.4.26
Chef Infra Client version: 15.3.14
Chef InSpec version: 4.16.0
Test Kitchen version: 2.3.3
Foodcritic version: 16.1.1
Cookstyle version: 5.6.2
/opt/chefdk/bin/chef --version  7.02s user 3.15s system 99% cpu 10.257 total

This breaks a ruby spec that doesn't seem to be useful at all where if you require a c-extension first before requiring the ruby extension that the ruby extension will not load.

There are some gems, including internal ones which do have both .so and .rb files like openssl.rb/openssl.so -- but in those cases they load the ruby file first and then load the .so, so this patch does not break that behavior.

The spec dates back over 10 years to the inception of the ruby/spec project with no documentation as to what the use case / design consideration actually was.

@lamont-granquist
Copy link
Contributor Author

and validated my thinking against @zenspider and he couldn't come up with a problem with this either.

tas50
tas50 reviewed Nov 2, 2019
View reviewed changes
config/software/ruby.rb Outdated Show resolved Hide resolved
@lamont-granquist
accelerate core ruby requires
57a1f69 
see https://bugs.ruby-lang.org/issues/15856

Signed-off-by: Lamont Granquist <lamont@scriptkiddie.org>
This was referenced Nov 7, 2019
Closed
Closed
@lamont-granquist lamont-granquist merged commit 8381e66 into master Nov 13, 2019
@lamont-granquist lamont-granquist deleted the lcg/accel-requires branch November 13, 2019 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tas50 tas50 tas50 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lamont-granquist @tas50